Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01269AjGvnGyzCa3USYaTjbU
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01269AjGvnGyzCa3USYaTjbU
dkoo
left a comment
dkoo
left a comment
There was a problem hiding this comment.
Release risk: LOW. Clean, well-documented change. The transient sentinel handling (false vs '0') is correct, the cache-invalidation hook set matches the detection query's predicates (status + placement meta), the editor-notice useEffect cleanup leaves no stale notices, and the new tests exercise the real production invalidation paths rather than a test-only flush. The has_published_above_header_prompts() docblock is unusually candid about both the coarse-detection trade-off and the page-cache staleness limitation.
One correctness gap is worth addressing before merge: the JS-delay exclusion only removes the reveal scripts from Newspack's own default contribution, not from a publisher's already-persisted delay_js_inclusions, so on sites that have saved Perfmatters' JS-delay settings through its UI the fix can silently no-op. The two other inline notes are optional hardening. Details inline.
chickenn00dle
left a comment
chickenn00dle
left a comment
There was a problem hiding this comment.
Release risk: LOW. Clean, well-documented change and everything runtime-checkable passes — the merged-list array_diff correctly subtracts the reveal scripts (including from a publisher's already-saved delay list while preserving their own entries), the request memo issues zero repeat queries, the re-entrancy guard and transient '0' sentinel are both correct, cache invalidation fires on status transition / placement-meta / delete / trash, and both plugins' new tests are green.
Nothing blocking. One thing to handle before merge and three optional cleanups:
- Merge conflict with
main. GitHub shows this PR as conflicting: anadd/addconflict ontests/unit-tests/perfmatters.php.class-perfmatters.phpauto-merges cleanly, somain's newer WooCommerce-scripts veto (NPPM-193) source survives — but its three WC-veto tests live only inmain's copy of that test file. Resolving the conflict by keeping this branch's version wholesale would silently drop them and leave that method untested; union both test sets when you rebase. - Optional hardening: a
try/finallyso an exception mid-query can't leave the re-entrancy flag stucktruefor the rest of the request (fail-safe if skipped),'orderby' => 'none'on the existence query to skip a filesort, and an assertion pinning thatwindow.newspackstays out of the defer exclusions.
| @@ -0,0 +1,107 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Severity: suggestion
GitHub reports this PR as CONFLICTING. The merge-base predates NPPM-193 (the Perfmatters WooCommerce-scripts veto), which landed on main afterward and created this same test file with three WC-veto tests (test_vetoes_when_wc_content_present, test_passes_through_when_no_wc_content, test_ignore_defaults_passes_through). This branch independently created the file with the five above-header tests, so it's an add/add conflict. class-perfmatters.php auto-merges cleanly, so the veto source (maybe_keep_woocommerce_assets) survives — but those three tests live only in main's copy of this file, and resolving the conflict by keeping this branch's version wholesale would silently drop them, leaving the surviving method untested. When you rebase/merge main, union the two sets of tests rather than overwriting.
There was a problem hiding this comment.
Confirmed the diagnosis: NPPM-193 (commit 5aa59b0) is on origin/main but not in this branch, so neither maybe_keep_woocommerce_assets nor the three WC-veto tests exist here yet. This is genuinely a merge to resolve, not something I can fix in-place – hand-adding the WC-veto tests to this file would fail (the veto method is absent on this branch) and would still collide on the eventual merge. Flagging for a human to run the git merge origin/main and union both test sets (the five above-header tests here + the three WC-veto tests from main) into one class. Leaving this thread open.
dkoo
left a comment
dkoo
left a comment
There was a problem hiding this comment.
@adekbadek thanks for addressing the initial review feedback! Looks like our reviewer skill crossed paths with another review from @chickenn00dle—it'd be interesting to see if any of the feedback from this second review surfaced anything the first review missed. Redundancy is never a bad thing when it comes to code reviews 😄
From my perspective, the PR is approved.
Re-reviewed the follow-up commit (11523f45) against my earlier notes.
Release risk: LOW.
The delay-list fix is correct: the reveal scripts are now subtracted from the merged delay_js_inclusions (persisted option + Newspack's own scripts_to_delay()), so they're stripped even on a site whose saved Perfmatters settings already round-tripped them into storage — the case my earlier comment flagged and the old test masked. test_reveal_scripts_undelayed_when_already_in_saved_option covers exactly that non-empty-option path, and the negative test confirms the list is preserved when no above-header prompts exist. The defer/js_exclusions path is purely additive (excluding a script from deferral has no re-add hazard), so it correctly needs no equivalent subtraction.
The request-level memo + re-entrancy guard in has_published_above_header_prompts() is sound: the memo is written only on the outer pass, the nested re-entrant read returns false without poisoning it, and flush_above_header_cache() clears the memo so in-request invalidation via the save/meta/status/delete hooks still works.
Both of my other earlier points are addressed too — the memoization is in place, and the meta-hook scope is justified in the added comment. Approving. One optional nit inline; nothing blocking.
# Conflicts: # plugins/newspack-plugin/tests/unit-tests/perfmatters.php
|
@chickenn00dle Re-requesting review – the changes you asked for have been pushed and need a look before this merges:
|
github-actions
Bot
added
[Status] Approved
All Submissions:
Changes proposed in this Pull Request:
Above-header Campaign Prompts were appearing only after a multi-second delay on sites running Perfmatters, because the Newspack Perfmatters defaults delay the prompt's view script until the reader's first interaction. The script reveals the prompt (it strips the server-rendered
hiddenclass), so until it runs the prompt is invisible — hurting both visibility and CLS. Reported by CT Mirror and Outlier Media (NPPM-2934).This PR:
newspack-popups,window.newspack,newspack-plugin) from the JS delay queue and addsnewspack-popups/newspack-pluginto the defer exclusions. Every other Newspack script stays delayed, so the rest of the site keeps the performance benefit. Detection isNewspack_Popups_Model::has_published_above_header_prompts()— transient-cached (the filter runs on essentially every front-end request) and invalidated when a prompt is saved, transitions status, has itsplacementmeta changed, or is deleted.The reveal-script set is intentionally minimal (only what gates the prompt becoming visible) rather than un-delaying all Newspack JS. Detection deliberately keys on published + above_header placement only, not full display eligibility (active campaign group / activation window / segment match) — replicating the Inserter's per-request logic on this hot path is too expensive, and the tradeoff is fail-safe: a published-but-never-shown prompt only forfeits the JS-delay optimization, it breaks nothing. This is documented at the detection method.
Closes NPPM-2934.
How to test the changes in this Pull Request:
newspack-popups/window.newspackare no longer in Perfmatters' "Delay JavaScript" list and thatnewspack-popups/newspack-pluginare in the "Exclude from Delay/defer" set (or inspect viaapply_filters( 'option_perfmatters_options', [] )).Other information:
AboveHeaderDetectionTestin newspack-popups;Newspack_Test_Perfmattersin newspack-plugin.)